Skip to content
This repository has been archived by the owner on Apr 7, 2020. It is now read-only.

Allow region-ized fields in OpenStack CloudProfileConfig #482

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Allow region-ized fields in OpenStack CloudProfileConfig #482

merged 1 commit into from
Jan 7, 2020

Conversation

rfranzke
Copy link
Contributor

@rfranzke rfranzke commented Dec 4, 2019 •
edited
Loading

What this PR does / why we need it:
Allow region-ized fields in OpenStack CloudProfileConfig. Some OpenStack environments have dedicated keystone URLs, floating pools or load balancer per region/data center. Today, you would have to create a dedicated CloudProfile per region (although the backing cloud provider is the same). That's unfortunate. With this PR, it's possible to configure regions for the mentioned properties. This will allow to have only one CloudProfile like for the hyperscalers.

Special notes for your reviewer:

  • /cc @afritzler
  • On purpose, I didn't change the type of KeystoneURL from string to *string (because it's optional now) to prevent vendoring issues. We could do that later if we want.
# github.com/gardener/gardener/pkg/apis/core/v1alpha1
vendor/github.com/gardener/gardener/pkg/apis/core/v1alpha1/conversions.go:580:34: cannot use cloudProfileConfig.KeyStoneURL (type *string) as type string in assignment
# github.com/gardener/gardener/pkg/apis/core/v1beta1
vendor/github.com/gardener/gardener/pkg/apis/core/v1beta1/conversions.go:507:34: cannot use cloudProfileConfig.KeyStoneURL (type *string) as type string in assignment
make: *** [start-provider-openstack] Error 2

Release note:

The OpenStack `CloudProfileConfig` now features a new `keyStoneURLs` field. It is a mapping of regions to keystone URLs. Also, the `constraints.loadBalancerProviders` and `constraints.floatingPools` fields now can optionally have a `region` property. This allows to only have one `CloudProfile` for an OpenStack environment in which the keystone URL, floating pools, and/or load balancer providers differ per region. See [this document](https://github.com/gardener/gardener-extensions/blob/master/controllers/provider-openstack/docs/usage-as-operator.md#cloudprofileconfig) for an example resource. :important: Please note that these fields will only be usable with gardener/gardener once the deprecated `garden.sapcloud.io/v1beta1` API group has been removed.

@rfranzke rfranzke requested a review from a team as a code owner December 4, 2019 19:30
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 4, 2019
@rfranzke rfranzke requested a review from afritzler December 6, 2019 05:54
afritzler
afritzler reviewed Dec 6, 2019
View reviewed changes
Copy link
Contributor

@afritzler afritzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I wanted to do some testing next week.

@rfranzke
Copy link
Contributor Author

rfranzke commented Dec 9, 2019

/cc @gardener/dashboard-maintainers - PTAL

timuthy
timuthy reviewed Dec 9, 2019
View reviewed changes
Copy link
Contributor

@timuthy timuthy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR! I have a general comment about the setting override, though.
For the keyStoneURL the non region specific setting overrides the region specifics and for LoadBalancerProvider and FloatingPool the order of elements determines how it is overwritten. I think this should be a) consistently handled and b) the region specifics should overrule the non-region specifics. WDYT?

controllers/provider-openstack/pkg/apis/openstack/validation/cloudprofile.go Show resolved Hide resolved
controllers/provider-openstack/pkg/controller/controlplane/valuesprovider.go Outdated Show resolved Hide resolved
controllers/provider-openstack/pkg/controller/controlplane/valuesprovider.go Outdated Show resolved Hide resolved
controllers/provider-openstack/pkg/controller/worker/machines.go Outdated Show resolved Hide resolved
@grolu grolu mentioned this pull request Dec 9, 2019
Closed
@rfranzke
Copy link
Contributor Author

As discussed with @timuthy I will adapt the PR so that it's allowed to specify both the non-regional and regional fields (e.g., keystoneURL and keystoneURLs). The logic will always prefer a regional setting if found, if not it will fallback to the non-regional setting.

@rfranzke rfranzke requested a review from timuthy December 30, 2019 14:59
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 30, 2019
@ialidzhikov ialidzhikov added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 3, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 3, 2020
@ialidzhikov
Copy link
Contributor

Looks good to me.
Nit - rebase needed because of the new year.

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 7, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 7, 2020
@rfranzke
Copy link
Contributor Author

rfranzke commented Jan 7, 2020

Thanks, merging after rebase.

@rfranzke rfranzke merged commit 8dad23f into gardener-attic:master Jan 7, 2020
@rfranzke rfranzke deleted the feature/openstack branch January 7, 2020 08:00
@afritzler afritzler mentioned this pull request Jan 30, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@afritzler afritzler afritzler left review comments

@timuthy timuthy Awaiting requested review from timuthy

Assignees
No one assigned
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rfranzke @ialidzhikov @afritzler @timuthy @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3